Fix resource wrapper method name collisions when segment equals operation ID - #61
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR adds a new OpenAPI spec for LM Studio’s local REST API and updates the Zig generator to avoid resource-wrapper naming collisions by emitting aliases. It also adds a test fixture that exercises the collision case. ChangesLM Studio OpenAPI Specification
API Generator Alias Handling
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
christianhelle
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/generators/unified/api_generator.zig (1)
979-986: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winConfirm intentional split between rename-only vs. alias-needed collisions.
This correctly extends collision detection to ancestor/sibling declaration names, but only sets
collides(renaming) — it never setsneeds_alias. As noted on Line 913-920, if a wrapper'soperation_idmatches one of the ancestor names captured here, the resulting rename alone is insufficient; the call site still needs the module-level alias to avoid referencing the shadowed name.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/generators/unified/api_generator.zig` around lines 979 - 986, The collision detection in the wrapper handling path only marks `collides` and misses the alias requirement for shadowing cases. Update the collision logic in `api_generator.zig` around the wrapper name checks so that when a wrapper’s `operation_id` or method name matches an ancestor/sibling declaration captured in `declarations` and `wrappers`, it also sets `needs_alias` in addition to `collides`. Keep the existing rename behavior, but ensure the code path that detects ancestor/child struct name collisions distinguishes rename-only from alias-needed cases.
🧹 Nitpick comments (1)
src/generators/unified/api_generator.zig (1)
852-957: 🚀 Performance & Scalability | 🔵 TrivialConsider adding unit tests for the new alias/collision logic.
This is subtle, generator-internal logic (struct-name vs. operation-id shadowing) with no accompanying test in this file. A small test constructing a synthetic
UnifiedDocumentwith a colliding path/operationId (mirroring the lmstudiochatcase) would guard against regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/generators/unified/api_generator.zig` around lines 852 - 957, Add unit coverage for the new alias/collision handling in generateResourceWrappers, since this is generator-internal shadowing logic. Create a synthetic UnifiedDocument that exercises the struct-name vs. operationId collision path (similar to the chat case), then assert the generated output includes the expected alias symbols and collision behavior from UnifiedApiGenerator/resourceMethodName/resourceAliasConflicts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openapi/v3.0/lmstudio.json`:
- Around line 987-997: The load_config schema currently uses oneOf with
LlmLoadConfig and EmbeddingLoadConfig, but EmbeddingLoadConfig is a subset of
LlmLoadConfig so the variants are not mutually exclusive. Update the load_config
definition in lmstudio.json to make the two schemas distinguishable, either by
adding a discriminator or by introducing a required field in one or both
referenced schemas. Ensure the chosen fix keeps the existing load_config shape
aligned with LlmLoadConfig and EmbeddingLoadConfig while preventing ambiguous
validation.
In `@src/generators/unified/api_generator.zig`:
- Around line 923-934: The alias emission in api_generator.zig is building
identifiers from wrapper.operation_id without the same sanitization used
elsewhere, so reserved words or special characters can generate invalid Zig.
Update the alias generation inside the wrapper loop to route both the alias name
and its target through the existing identifier-sanitizing helpers used by the
generator, such as appendIdentifier or sanitizeIdentifierAlloc, consistent with
how op_id is handled elsewhere. Keep the behavior in the wrapper.operation_id
branch and the hasReturnValue(wrapper.method, wrapper.operation) result alias
generation, but ensure both emitted names are escaped/sanitized before appending
to self.buffer.
- Around line 913-920: Alias detection in the wrapper generation logic is only
checking the final segment against wrapper.method_name, but the generated call
uses wrapper.operation_id, so nested resource name collisions can still miss
needs_alias. Update the collision test in the wrapper loop to compare
wrapper.operation_id against every entry in wrapper.segments, and derive both
collides and needs_alias from that same check so ancestor/child conflicts are
handled consistently.
---
Duplicate comments:
In `@src/generators/unified/api_generator.zig`:
- Around line 979-986: The collision detection in the wrapper handling path only
marks `collides` and misses the alias requirement for shadowing cases. Update
the collision logic in `api_generator.zig` around the wrapper name checks so
that when a wrapper’s `operation_id` or method name matches an ancestor/sibling
declaration captured in `declarations` and `wrappers`, it also sets
`needs_alias` in addition to `collides`. Keep the existing rename behavior, but
ensure the code path that detects ancestor/child struct name collisions
distinguishes rename-only from alias-needed cases.
---
Nitpick comments:
In `@src/generators/unified/api_generator.zig`:
- Around line 852-957: Add unit coverage for the new alias/collision handling in
generateResourceWrappers, since this is generator-internal shadowing logic.
Create a synthetic UnifiedDocument that exercises the struct-name vs.
operationId collision path (similar to the chat case), then assert the generated
output includes the expected alias symbols and collision behavior from
UnifiedApiGenerator/resourceMethodName/resourceAliasConflicts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: acd84aac-9efb-4b04-8e8a-ad5475c894b6
⛔ Files ignored due to path filters (7)
generated/generated_v2.zigis excluded by!**/generated/**generated/generated_v2_yaml.zigis excluded by!**/generated/**generated/generated_v3.zigis excluded by!**/generated/**generated/generated_v32.zigis excluded by!**/generated/**generated/generated_v3_yaml.zigis excluded by!**/generated/**generated/lmstudio.zigis excluded by!**/generated/**generated/main.zigis excluded by!**/generated/**
📒 Files selected for processing (2)
openapi/v3.0/lmstudio.jsonsrc/generators/unified/api_generator.zig
Comment on lines
+987
to
+997
| "load_config": { | ||
| "description": "The final configuration applied to the loaded model. Included only when echo_load_config is true", | ||
| "oneOf": [ | ||
| { | ||
| "$ref": "#/components/schemas/LlmLoadConfig" | ||
| }, | ||
| { | ||
| "$ref": "#/components/schemas/EmbeddingLoadConfig" | ||
| } | ||
| ] | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
python3 - <<'PY'
import json, re, pathlib, sys
path = pathlib.Path('openapi/v3.0/lmstudio.json')
data = json.loads(path.read_text())
schemas = data['components']['schemas']
for name in ['LlmLoadConfig', 'EmbeddingLoadConfig']:
print(f'### {name}')
schema = schemas[name]
print(json.dumps(schema, indent=2)[:4000])
print()
load_cfg = schemas['LoadModelResponse']['properties']['load_config']
print('### LoadModelResponse.load_config')
print(json.dumps(load_cfg, indent=2))
PYRepository: christianhelle/openapi2zig
Length of output: 1504
🏁 Script executed:
python3 - <<'PY'
import json, pathlib
path = pathlib.Path('openapi/v3.0/lmstudio.json')
data = json.loads(path.read_text())
schemas = data['components']['schemas']
for name in ['LlmLoadConfig', 'EmbeddingLoadConfig']:
print(f'### {name}')
print(json.dumps(schemas[name], indent=2))
print()
print('### LoadModelResponse.load_config')
print(json.dumps(schemas['LoadModelResponse']['properties']['load_config'], indent=2))
PYRepository: christianhelle/openapi2zig
Length of output: 1504
load_config needs a mutually exclusive schema. EmbeddingLoadConfig is a subset of LlmLoadConfig (context_length only), so a payload like {"context_length": 4096} matches both and breaks oneOf validation. Add a discriminator or another required field to separate the variants.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openapi/v3.0/lmstudio.json` around lines 987 - 997, The load_config schema
currently uses oneOf with LlmLoadConfig and EmbeddingLoadConfig, but
EmbeddingLoadConfig is a subset of LlmLoadConfig so the variants are not
mutually exclusive. Update the load_config definition in lmstudio.json to make
the two schemas distinguishable, either by adding a discriminator or by
introducing a required field in one or both referenced schemas. Ensure the
chosen fix keeps the existing load_config shape aligned with LlmLoadConfig and
EmbeddingLoadConfig while preventing ambiguous validation.
Comment on lines
+923
to
+934
| for (wrappers.items) |wrapper| { | ||
| if (wrapper.needs_alias) { | ||
| const alias_line = try std.fmt.allocPrint(self.allocator, "const _{s} = {s};\n", .{ wrapper.operation_id, wrapper.operation_id }); | ||
| defer self.allocator.free(alias_line); | ||
| try self.buffer.appendSlice(self.allocator, alias_line); | ||
| if (self.hasReturnValue(wrapper.method, wrapper.operation)) { | ||
| const result_line = try std.fmt.allocPrint(self.allocator, "const _{s}Result = {s}Result;\n", .{ wrapper.operation_id, wrapper.operation_id }); | ||
| defer self.allocator.free(result_line); | ||
| try self.buffer.appendSlice(self.allocator, result_line); | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Alias constant names/targets aren't sanitized/escaped like other emitted identifiers.
alias_line/result_line are built by directly interpolating wrapper.operation_id into the source text, bypassing appendIdentifier/sanitizeIdentifierAlloc, which is used everywhere else in this file for spec-derived identifiers (e.g. Line 1350: try self.appendIdentifier(op_id)). If operation_id is a Zig reserved word or contains characters requiring @"..." escaping, this produces invalid Zig source.
🐛 Proposed fix
- for (wrappers.items) |wrapper| {
- if (wrapper.needs_alias) {
- const alias_line = try std.fmt.allocPrint(self.allocator, "const _{s} = {s};\n", .{ wrapper.operation_id, wrapper.operation_id });
- defer self.allocator.free(alias_line);
- try self.buffer.appendSlice(self.allocator, alias_line);
- if (self.hasReturnValue(wrapper.method, wrapper.operation)) {
- const result_line = try std.fmt.allocPrint(self.allocator, "const _{s}Result = {s}Result;\n", .{ wrapper.operation_id, wrapper.operation_id });
- defer self.allocator.free(result_line);
- try self.buffer.appendSlice(self.allocator, result_line);
- }
- }
- }
+ for (wrappers.items) |wrapper| {
+ if (wrapper.needs_alias) {
+ try self.buffer.appendSlice(self.allocator, "const _");
+ try self.appendIdentifier(wrapper.operation_id);
+ try self.buffer.appendSlice(self.allocator, " = ");
+ try self.appendIdentifier(wrapper.operation_id);
+ try self.buffer.appendSlice(self.allocator, ";\n");
+ if (self.hasReturnValue(wrapper.method, wrapper.operation)) {
+ const operation_result_name = try std.fmt.allocPrint(self.allocator, "{s}Result", .{wrapper.operation_id});
+ defer self.allocator.free(operation_result_name);
+ try self.buffer.appendSlice(self.allocator, "const _");
+ try self.appendIdentifier(operation_result_name);
+ try self.buffer.appendSlice(self.allocator, " = ");
+ try self.appendIdentifier(operation_result_name);
+ try self.buffer.appendSlice(self.allocator, ";\n");
+ }
+ }
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/generators/unified/api_generator.zig` around lines 923 - 934, The alias
emission in api_generator.zig is building identifiers from wrapper.operation_id
without the same sanitization used elsewhere, so reserved words or special
characters can generate invalid Zig. Update the alias generation inside the
wrapper loop to route both the alias name and its target through the existing
identifier-sanitizing helpers used by the generator, such as appendIdentifier or
sanitizeIdentifierAlloc, consistent with how op_id is handled elsewhere. Keep
the behavior in the wrapper.operation_id branch and the
hasReturnValue(wrapper.method, wrapper.operation) result alias generation, but
ensure both emitted names are escaped/sanitized before appending to self.buffer.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/tests/resource_wrapper_tests.zig (2)
147-147: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering
.pathsmode too.The fixture's path (
/api/v1/chat) would also trigger the same last-segment collision under.pathsmode, so including it would close a coverage gap for this exact regression.♻️ Suggested addition
- const modes = [_]cli.ResourceWrapperMode{ .tags, .hybrid }; + const modes = [_]cli.ResourceWrapperMode{ .tags, .paths, .hybrid };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tests/resource_wrapper_tests.zig` at line 147, The ResourceWrapper mode coverage in the test fixture currently skips .paths, leaving the last-segment collision regression untested. Update the modes array in resource_wrapper_tests.zig to include .paths alongside .tags and .hybrid, and keep the existing fixture path (/api/v1/chat) so the same collision case is exercised through ResourceWrapper behavior.
145-168: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNew test uses
std.testing.allocatorinstead of the required test allocator pattern.As per coding guidelines,
**/tests/**/*.zigfiles should "Usetest_utils.createTestAllocator()in all tests for leak detection" with the patternvar gpa = test_utils.createTestAllocator(); const allocator = gpa.allocator(); defer document.deinit(allocator);. This new test (and the file overall) usesstd.testing.allocatordirectly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tests/resource_wrapper_tests.zig` around lines 145 - 168, The new test in resource_wrapper_tests.zig is using std.testing.allocator directly instead of the required leak-detecting test allocator pattern. Update this test to use test_utils.createTestAllocator() and obtain the allocator from it, following the same setup/teardown style used in other tests so allocations are tracked properly. Keep the existing document.deinit(allocator), generator lifecycle, and assertions unchanged, but make sure the test uses the shared test_utils allocator pattern consistently across this file.Source: Coding guidelines
src/generators/unified/api_generator.zig (1)
1246-1271: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBare
catch return true;without|err|/context.The new
operationDeclaresTopLevelNameswallows allocation failures viacatch return true;at Lines 1251, 1256, and 1264, without capturing|err|or a context message. As per coding guidelines,**/*.zigfiles should "Use Zig error sets and catch |err| pattern for error handling with meaningful context messages." (Mirrors an existing pattern at Line 1238, but this PR introduces new occurrences.)♻️ Example fix
- const raw_name = std.fmt.allocPrint(self.allocator, "{s}Raw", .{operation_id}) catch return true; + const raw_name = std.fmt.allocPrint(self.allocator, "{s}Raw", .{operation_id}) catch |err| { + std.log.warn("failed to allocate alias-conflict name for '{s}': {s}", .{ operation_id, `@errorName`(err) }); + return true; + };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/generators/unified/api_generator.zig` around lines 1246 - 1271, The new operationDeclaresTopLevelName helper is swallowing allocation failures with bare catch return true, which bypasses the required Zig error handling pattern. Update each allocPrint call in operationDeclaresTopLevelName to capture the error with |err| and add a meaningful context message before deciding how to proceed, matching the existing style used elsewhere in UnifiedApiGenerator. Keep the fallback behavior explicit, but do not leave the allocation failures unreported or context-free.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/generators/unified/api_generator.zig`:
- Around line 1246-1271: The new operationDeclaresTopLevelName helper is
swallowing allocation failures with bare catch return true, which bypasses the
required Zig error handling pattern. Update each allocPrint call in
operationDeclaresTopLevelName to capture the error with |err| and add a
meaningful context message before deciding how to proceed, matching the existing
style used elsewhere in UnifiedApiGenerator. Keep the fallback behavior
explicit, but do not leave the allocation failures unreported or context-free.
In `@src/tests/resource_wrapper_tests.zig`:
- Line 147: The ResourceWrapper mode coverage in the test fixture currently
skips .paths, leaving the last-segment collision regression untested. Update the
modes array in resource_wrapper_tests.zig to include .paths alongside .tags and
.hybrid, and keep the existing fixture path (/api/v1/chat) so the same collision
case is exercised through ResourceWrapper behavior.
- Around line 145-168: The new test in resource_wrapper_tests.zig is using
std.testing.allocator directly instead of the required leak-detecting test
allocator pattern. Update this test to use test_utils.createTestAllocator() and
obtain the allocator from it, following the same setup/teardown style used in
other tests so allocations are tracked properly. Keep the existing
document.deinit(allocator), generator lifecycle, and assertions unchanged, but
make sure the test uses the shared test_utils allocator pattern consistently
across this file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 666b07ad-8cfb-4231-b01b-109f193d2449
📒 Files selected for processing (2)
src/generators/unified/api_generator.zigsrc/tests/resource_wrapper_tests.zig
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix a compile error in generated lmstudio.zig where resource wrapper method names collided with their containing struct names (e.g., path segment
chatand operation IDchat).Changes
needs_aliasdetection inResourceWrapperwhen wrapper method name matches its containing struct nameconst _chat = chat;) for colliding operationsneeds_aliasis trueSummary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes